-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify usage of beforematch event #11151
base: main
Are you sure you want to change the base?
Conversation
This PR improves a paragraph which talks about firing the beforematch event by making it reference the revealing algorithm which actually fires it. It also sets the bubbles attribute to true, which wasn't specified before. Fixes whatwg#11150
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nit.
This is a normative change (because of the new bubbling behavior) so it would be good to fill out the template completely before merging. Hopefully it will be simple to get approval from @jnjaeschke that Mozilla supports this behavior as well.
data-x="event-beforematch">beforematch</code> at <var>currentNode</var>.</p></li> | ||
data-x="event-beforematch">beforematch</code> with the <code | ||
data-x="dom-Event-bubbles">bubbles</code> attribute initialized to true at | ||
<var>currentNode</var>.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the order should be "Fire an event named e at target with..." per https://dom.spec.whatwg.org/#firing-events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the order of operations here, I guess we have a test that asserts the hidden
attribute is still present?
If the event listener removes the attribute I guess the next step is a no-op, though the way it's worded makes that a bit unclear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the order should be "Fire an event named e at target with..." per https://dom.spec.whatwg.org/#firing-events
Done
If the event listener removes the attribute I guess the next step is a no-op, though the way it's worded makes that a bit unclear.
Do you have any suggestions to change the wording?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we referenced an actual "remove an attribute" primitive (assuming there is one) it would probably be clearer, but it doesn't have to be fixed for this PR.
This is not editorial, and the relevant API is part of interop-25. I didn't immediately find a test checking that hidden attribute is still present, but maybe it is checked in some other test? |
Ok, I removed the sentence about editorialness from the commit message.
I didn't find any tests, so I will add one |
This PR improves a paragraph which talks about firing the beforematch event by making it reference the revealing algorithm which actually fires it. It also sets the bubbles attribute to true, which wasn't specified before.
Fixes #11150
(See WHATWG Working Mode: Changes for more details.)
/interaction.html ( diff )